Skip to content

Add count and ordering to json_asserter and non-vnd error reports#25

Merged
mlclay merged 4 commits intomasterfrom
feature/json-asserter-count-ordering
Mar 3, 2020
Merged

Add count and ordering to json_asserter and non-vnd error reports#25
mlclay merged 4 commits intomasterfrom
feature/json-asserter-count-ordering

Conversation

@jamesfneyer
Copy link
Copy Markdown
Contributor

This adds two features the json_asserter for 200 checks, count and check_ordering. This allows two things: ensuring the amount of data returned in the list matches the amount expected, and allowing for a precise ordering check on returning data. This allows for better testing of filter/search fields objects, as it can ensure that only the data expected was returned.

In addition, the logic for asserting errors in the 400 range did not allow for checking non-vnd errors. This is an issue in the telemetry branch on Transmission, as the response is not vnd and therefore cannot be checked in the normal way, leading to inconsistencies in testing.

@jamesfneyer jamesfneyer requested a review from mlclay February 24, 2020 21:27
@ajhodges
Copy link
Copy Markdown
Contributor

"In addition, the logic for asserting errors in the 400 range did not allow for checking non-vnd errors. This is an issue in the telemetry branch on Transmission, as the response is not vnd and therefore cannot be checked in the normal way, leading to inconsistencies in testing."

Isn't the json_asserter code mostly for making assertions about JSON API data? And not just plain json? IDR @mlclay

@jamesfneyer
Copy link
Copy Markdown
Contributor Author

We have the logic for checking plain JSON successful responses, so it seemed logical to also have an option to check plain JSON failing responses.

@mlclay
Copy link
Copy Markdown
Contributor

mlclay commented Mar 2, 2020

The json_asserter was initially intended for JSON API data; however, during initial development, I came across a few situations where testing standard JSON responses was helpful. There is already a basic level of support for plain JSON responses by providing vnd=False to the methods. I think this expansion to plain JSON error responses is within reason.

Copy link
Copy Markdown
Contributor

@mlclay mlclay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change, nice work.

For consistency in the code I have have a recommendation:

Currently the top level success assertions call the intermediate method response_has_data which then calls _test_vnd_json or _test_regular_json based on the value of vnd. This intermediate method is very small, but it does encapsulate the conditional checks to a single location.

However, the error assertion methods have the conditional calls directly to response_has_error or response_has_json_error (also based on vnd). There is no intermediate method to centralize the checking of the conditions for branching to different assertion logic.

I recommend the error response logic to follow the same pattern that exists in the success response methods. This will keep code readability higher as there is consistency between the methods and keeps the code DRYer as the conditional checks are in a single method instead of spread across each top-level error assertion method.

Comment thread src/shipchain_common/test_utils/json_asserter.py Outdated
@jamesfneyer jamesfneyer requested a review from mlclay March 2, 2020 18:22
Copy link
Copy Markdown
Contributor

@mlclay mlclay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mlclay mlclay merged commit f7c23c9 into master Mar 3, 2020
@mlclay mlclay deleted the feature/json-asserter-count-ordering branch March 3, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants